I think thats good, but need someone else's eyes on this to confirm.

Can you describe and what's the changing here?

suite.SupportedFeatures is created from options.SupportedFeatures and when we are updating suite.SupoortedFeatures (which will be used for testing) we are checking if the proper features were populated in the options.SupportedFeatures, we don't want simultaneously check and update the same set, as it makes logic vulnerable to some obscure bugs.

P.S. also can you please add unit tests for the fixed logic.

@bexxmodd the bug is described in #4112.

The issue occurs when supported features are inferred from the GatewayClass, in those cases, they aren’t included in options.SupportedFeatures but only set in suite.SupportedFeatures. As a result, the conformance test report incorrectly marks them as unsupported.

Discussed this with @LiorLieberman offline. I see the issue.

Just a nit: Can you please use supportedFeatures for check, instead of suite.SupportedFeatures ? As in above comment, it's not a great practice to update the set that is also checked upon in the loop.

But then we’d also have to change:

			if !suite.SupportedFeatures.Has(f) {
				suite.SupportedFeatures.Insert(f)
			}

to:

			if !supportedFeatures.Has(f) {
				suite.SupportedFeatures.Insert(f)
			}

That doesn’t seem right to me. I see supportedFeatures as a temporary variable that should only be used until the suite is initialized.

For unit test coverage, I’d propose adding assertions for extendedSupportedFeatures in TestInferGWCSupportedFeatures.

My bad. I meant to use supportedFeatures to check if it has a name or not.

@bexxmodd I’m not sure I understand what you’re requesting, could you provide a code suggestion?

if !supportedFeatures.Has(f) {
    suite.SupportedFeatures.Insert(f)
}

It's a temporary variable until final supported features are determined for testing and NewConformanceTestSuite returns ConformanceTestSuite. Nothing limits it's scope until suite is initialized. There's no scope conflict or a misleading name so I don't see why it should go out of usage after suite is initialized. it's the sound representation of populated features from the Status field.

In my opinion, it doesn’t make sense to use supportedFeatures after it has already been assigned to suite.SupportedFeatures. Modifying it at that point would even be incorrect, why should we still rely on it?

In case additional code was inserted above this section that changed suite.SupportedFeatures, then the logic here is supposed to use the actual (possibly modified) features of the suite (suite.SupportedFeatures) rather than the originally derived ones in supportedFeatures.

If it is important that supportedFeatures is used in this part, I would suggest moving the entire section before the initialization of the ConformanceTestSuite object.

It is actually a really good idea to move the whole section before initializing the ConformanceTestSuite.

@bexxmodd could you take a close look at the refactoring? I simplified the logic quite a bit.

This is way cleaner! Thanks Norwin!

This is way cleaner! Thanks Norwin!